New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support library fragment watcher #1611
Add support library fragment watcher #1611
Conversation
This looks useful to me; @pyricau what do you think? Quick (somewhat related) question: @AndreasBoehm do you see AndroidX dependencies being pulled into project when using LeakCanary? I'm wondering if developers who still use Support Lib end up pulling the AndroidX classes via transitive dependency through LeakCanary. |
@@ -0,0 +1,58 @@ | |||
/* | |||
* Copyright (C) 2018 Square, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2019 ;) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy & paste 🤦♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
if (classAvailable(ANDROID_SUPPORT_FRAGMENT_CLASS_NAME) && | ||
classAvailable(ANDROID_SUPPORT_FRAGMENT_DESTROY_WATCHER_CLASS_NAME) | ||
) { | ||
val watcherConstructor = Class.forName(ANDROID_SUPPORT_FRAGMENT_DESTROY_WATCHER_CLASS_NAME) | ||
.getDeclaredConstructor(ObjectWatcher::class.java, Function0::class.java) | ||
@kotlin.Suppress("UNCHECKED_CAST") | ||
fragmentDestroyWatchers.add( | ||
watcherConstructor.newInstance(objectWatcher, configProvider) as (Activity) -> Unit | ||
) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block can probably be extracted to a method, so we can reuse it in previous if
statement for ANDROIDX_FRAGMENT_CLASS_NAME
- this will remove the code duplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually wanted to do this but forget about it after everything worked as expected. Sorry!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed within my latest commit
build.gradle
Outdated
@@ -34,6 +34,7 @@ buildscript { | |||
mockito_kotlin: 'com.nhaarman:mockito-kotlin-kt1.1:1.5.0', | |||
okio: 'com.squareup.okio:okio:2.2.2', | |||
robolectric : 'org.robolectric:robolectric:4.0-alpha-3', | |||
android_support: 'com.android.support:support-v4:28.0.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this line to be after androidx
- it's (more-or-less) alphabetically sorted now 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved :)
Can you also add this module here: https://github.com/square/leakcanary/blob/master/leakcanary-android-core/build.gradle#L7 Essentially this will remove the need to add Side note: we should make sure this doesn't cause issues with jetifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving (a few changes still needed but overall looking good)
Thanks for accepting my PR! I would love to adjust it right now but i am on holidays and i don’t have my computer with me. I can adjust it in roughly two weeks if this is fine for you? I wasn’t sure about adding the support library watcher to the core package as i didn’t want to add code for a legacy library to the project. But i agree that it would make things easier for users of the library. |
Thats a good question! |
I'm ok with waiting, would love to give you the opportunity to finish this. Once this is ready we should probably use it on a jetified project and make sure Gradle won't complain. |
Thank you! Will work on it after my holidays. Talk to you soon. |
9a0961d
to
c6b3a47
Compare
Hi @pyricau, I addressed your comments but I was not able to test the library on a jetified project. @Armaxis I will check later today if leakcanary pulled in AndroidX dependencies into my project. |
Try this (changing the path to your local repo):
|
Updated the docs to document this: https://square.github.io/leakcanary/dev-env/#deploying-locally |
I checked out the branch, deployed locally, added the support library only to a project then support library + jetifier and it's all been working, no issue. I'm going to merge :) |
Because there are some projects that can't migrate to AndroidX yet and my main project is one of those I figured that it might make sense to share this code with the community to allow everyone to use leakcanary 2 with the good old support library.
To watch leaking support library fragments and their views the following needs to be added to the build.gradle.
To test this PR you need to add the support-fragments project to the build.gradle.